-
Notifications
You must be signed in to change notification settings - Fork 515
requireDotNotation: Require dots for es3 keywords when not in es3 mode. #825
Conversation
/cc @mdevils, @markelog, @mrjoelkemp review please? |
var file = new JsFile(filename, str, tree); | ||
|
||
var file = new JsFile(filename, str, tree, { | ||
es3: this._configuration.isES3Enabled(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this an options hash because I plan on also making tabSize a file level option and didn't want to go adding more and more formal parameters.
Hello, Mike. Good job on this PR. In this implementation I can see |
@mdevils I toyed around with a single variable for which versions we need to target, and started off with your suggestion (I even had a Are there any cases where multiple discrete values matter? If not, I can go back to a single value. |
I don't think so. |
@mdevils consider the following code:
Should both If it's the latter, and we forgo the ability to target only es3, then yes, I can get away with |
Nevermind, it's nonsensical to say my code ONLY supports IE6. I'll switch it to a number. |
@mikesherov, thank you 😉 |
Probably |
Just for the record, the only API I'm changing here is the one in JsFile, right @mdevils ? |
@zxqfox in this case, |
@mikesherov You mean |
Mike, I have one more idea. What if we introduce new class And then rules would just use it: So we completely encapsulate this in a class and in the future we can even introduce dialects like |
I'm agreed with @mdevils. It's pretty nice to have something like that and I hope there's no troubles with implementing this ;-) |
OK, So I'll introduce the |
Thanks for the input folks! |
😄 Great 👍 |
CLI options are OK for me currently, Mike. It's only about JsFile 😉. |
@mdevils although, it isn't that But basically it means that the default What's cool though is now all the |
@mikesherov are you sure I've seen it here: http://www.ecma-international.org/ecma-262/5.1/#sec-12.10 |
I like |
It's not that it's not a keyword, it's that keywords are allowed as property names in ES5 |
@mikesherov, oh, OK. |
-1 from me, es3 environment is dying but this stuff complicates our cli interface and rules configuration. Not that my "-1" is a blocker more just a note but i feel uneasy with it. |
It does not complicate rule configuration, as it's not a rule configuration option, but rather a file option. We will need the idea of I think honestly that for the case of correctness, until ie8 is really dead, we need to consider es3. |
I agree with @mikesherov. Currently most websites respect ES3 due to IE8 JS implementation. |
Yep, this understandable and regrettable at the same time |
:-/ I'll try to keep the impact on the design of the code to a minimum. At least in 2.0 we'll make es3 off by default. |
Is this ready? |
@@ -15,6 +15,7 @@ program | |||
.usage('[options] <file ...>') | |||
.option('-c, --config [path]', 'configuration file path') | |||
.option('-e, --esnext', 'attempts to parse esnext code (currently es6)') | |||
.option('-t, --es3', 'validates code as es3') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Option -t
looks strange. What if we just remove short variant: .option('--es3', 'validates code as es3')
?
I believe --es3
is short enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair enough, I'll do that then.
I believe this is best for the future. Later we can introduce many JS-dialects support like |
Yes, I agree we need to support different variants, however, it's going to be hard to predict which pieces of functionality are important to abstract here. I believe using conditional logic for specific rules by querying what dialect you're using is going to be less overhead in the long run than introducing a class per dialect with explicit methods. To use this as a concrete example, I would consider it better to say In this case, I prefer conditional over inheritance and don't see ample proof that the long term will see benefits from introducing another set of classes. Thoughts? |
OK, Mike, lets postpone this abstraction layer until we face considerable amount of rules with dialect-specific behaviour. |
Thanks for compromising! I'm sure I'll regret my suggestion in the future! I'll update this PR now. |
cab9f35
to
520bfcc
Compare
@mdevils can you re-review? |
}); | ||
|
||
it('should not report dot notation', function() { | ||
assert(checker.checkString('var x = a.b').isEmpty()); | ||
describe('true value with es3 explicitly enabled', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm following the code paths properly, the es3
flag is implicitly true by default. Should the it
s for this describe be moved up a level since this is the default behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
es3 will be off by default in 2.0. I purposefully excluded it from the default behavior test so I don't have to change it in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice
LGTM |
Made a final review. Everything looks cool. Good job! |
Didn't review it, but i'm gonna merge it, since this is blocking #868 |
No description provided.